-
Notifications
You must be signed in to change notification settings - Fork 4.1k
opt: fix PruneUnionAllCols panic with outer scope columns #160580
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Before this change, the PruneUnionAllCols normalization rule would panic in crdb-test builds when the projection above a UnionAll referenced columns from an outer scope (e.g., due to an apply-join or routine). This occurred because the rule computed the needed column set by combining ProjectionOuterCols and passthrough columns, which could include outer scope columns not present in the UnionAll's output. These outer columns were then passed to NeededColMapLeft/Right, which call TranslateColSetStrict and panic when given unknown columns. This change fixes the issue by intersecting the needed column set with the UnionAll's output columns before passing it to NeededColMapLeft/ Right. This ensures only columns actually present in the UnionAll are translated, preventing the panic. Fixes cockroachdb#159793 Release note: None Co-Authored-By: Claude <[email protected]>
michae2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michae2 reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball).
|
TFTR! bors r=michae2 |
160580: opt: fix PruneUnionAllCols panic with outer scope columns r=michae2 a=DrewKimball Before this change, the PruneUnionAllCols normalization rule would panic in crdb-test builds when the projection above a UnionAll referenced columns from an outer scope (e.g., due to an apply-join or routine). This occurred because the rule computed the needed column set by combining ProjectionOuterCols and passthrough columns, which could include outer scope columns not present in the UnionAll's output. These outer columns were then passed to NeededColMapLeft/Right, which call TranslateColSetStrict and panic when given unknown columns. This change fixes the issue by intersecting the needed column set with the UnionAll's output columns before passing it to NeededColMapLeft/ Right. This ensures only columns actually present in the UnionAll are translated, preventing the panic. Fixes #159793 Release note: None Co-Authored-By: Claude <[email protected]> Co-authored-by: Drew Kimball <[email protected]>
|
Build failed: |
|
courtesy merge bors retry |
160580: opt: fix PruneUnionAllCols panic with outer scope columns r=michae2 a=DrewKimball Before this change, the PruneUnionAllCols normalization rule would panic in crdb-test builds when the projection above a UnionAll referenced columns from an outer scope (e.g., due to an apply-join or routine). This occurred because the rule computed the needed column set by combining ProjectionOuterCols and passthrough columns, which could include outer scope columns not present in the UnionAll's output. These outer columns were then passed to NeededColMapLeft/Right, which call TranslateColSetStrict and panic when given unknown columns. This change fixes the issue by intersecting the needed column set with the UnionAll's output columns before passing it to NeededColMapLeft/ Right. This ensures only columns actually present in the UnionAll are translated, preventing the panic. Fixes #159793 Release note: None Co-Authored-By: Claude <[email protected]> 160632: sql/bulkmerge: reuse SST iterator across bulk merge tasks r=spilchen a=spilchen This change reduces overhead in the bulk merge processor by initializing a single iterator over all input SSTs at startup, rather than creating a new one per task. The iterator is reused across tasks, seeking only when needed. Informs #159414 Epic: CRDB-48845 Release note: none Co-authored by: `@jeffswenson` Co-authored-by: Drew Kimball <[email protected]> Co-authored-by: Matt Spilchen <[email protected]>
|
Build failed (retrying...): |
160580: opt: fix PruneUnionAllCols panic with outer scope columns r=michae2 a=DrewKimball Before this change, the PruneUnionAllCols normalization rule would panic in crdb-test builds when the projection above a UnionAll referenced columns from an outer scope (e.g., due to an apply-join or routine). This occurred because the rule computed the needed column set by combining ProjectionOuterCols and passthrough columns, which could include outer scope columns not present in the UnionAll's output. These outer columns were then passed to NeededColMapLeft/Right, which call TranslateColSetStrict and panic when given unknown columns. This change fixes the issue by intersecting the needed column set with the UnionAll's output columns before passing it to NeededColMapLeft/ Right. This ensures only columns actually present in the UnionAll are translated, preventing the panic. Fixes #159793 Release note: None Co-Authored-By: Claude <[email protected]> Co-authored-by: Drew Kimball <[email protected]>
|
Build failed: |
|
courtesy merge 2 bors retry |
160580: opt: fix PruneUnionAllCols panic with outer scope columns r=michae2 a=DrewKimball Before this change, the PruneUnionAllCols normalization rule would panic in crdb-test builds when the projection above a UnionAll referenced columns from an outer scope (e.g., due to an apply-join or routine). This occurred because the rule computed the needed column set by combining ProjectionOuterCols and passthrough columns, which could include outer scope columns not present in the UnionAll's output. These outer columns were then passed to NeededColMapLeft/Right, which call TranslateColSetStrict and panic when given unknown columns. This change fixes the issue by intersecting the needed column set with the UnionAll's output columns before passing it to NeededColMapLeft/ Right. This ensures only columns actually present in the UnionAll are translated, preventing the panic. Fixes #159793 Release note: None Co-Authored-By: Claude <[email protected]> Co-authored-by: Drew Kimball <[email protected]>
|
Build failed:
|
|
courtesy merge 3 bors retry |
160580: opt: fix PruneUnionAllCols panic with outer scope columns r=michae2 a=DrewKimball Before this change, the PruneUnionAllCols normalization rule would panic in crdb-test builds when the projection above a UnionAll referenced columns from an outer scope (e.g., due to an apply-join or routine). This occurred because the rule computed the needed column set by combining ProjectionOuterCols and passthrough columns, which could include outer scope columns not present in the UnionAll's output. These outer columns were then passed to NeededColMapLeft/Right, which call TranslateColSetStrict and panic when given unknown columns. This change fixes the issue by intersecting the needed column set with the UnionAll's output columns before passing it to NeededColMapLeft/ Right. This ensures only columns actually present in the UnionAll are translated, preventing the panic. Fixes #159793 Release note: None Co-Authored-By: Claude <[email protected]> Co-authored-by: Drew Kimball <[email protected]>
|
Build failed: |
Before this change, the PruneUnionAllCols normalization rule would panic in crdb-test builds when the projection above a UnionAll referenced columns from an outer scope (e.g., due to an apply-join or routine). This occurred because the rule computed the needed column set by combining ProjectionOuterCols and passthrough columns, which could include outer scope columns not present in the UnionAll's output. These outer columns were then passed to NeededColMapLeft/Right, which call TranslateColSetStrict and panic when given unknown columns.
This change fixes the issue by intersecting the needed column set with the UnionAll's output columns before passing it to NeededColMapLeft/ Right. This ensures only columns actually present in the UnionAll are translated, preventing the panic.
Fixes #159793
Release note: None
Co-Authored-By: Claude [email protected]